-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove iteratorFoldM fixes #1716 #1740
remove iteratorFoldM fixes #1716 #1740
Conversation
@@ -214,17 +209,23 @@ class FoldableTestsAdditional extends CatsSuite { | |||
} | |||
|
|||
test(".foldLeftM short-circuiting optimality") { | |||
// test that no more elements are evaluated than absolutely necessary | |||
// test that no more than 1 elements are evaluated than absolutely necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we look if we can solve this additional evaluation with @TomasMikula's suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to update this comment. Fixed it already 👍
Codecov Report
@@ Coverage Diff @@
## master #1740 +/- ##
==========================================
- Coverage 93.99% 93.99% -0.01%
==========================================
Files 253 253
Lines 4180 4179 -1
Branches 155 85 -70
==========================================
- Hits 3929 3928 -1
Misses 251 251
Continue to review full report at Codecov.
|
weird, for some reason github doesn't allow me to request @TomasMikula to review. |
👍 However, it's still the case that most data structures can have a more efficient implementation of |
@TomasMikula yes, it's nice that you also mentioned it in the comment of |
@@ -90,9 +90,6 @@ trait ListInstances extends cats.kernel.instances.ListInstances { | |||
|
|||
override def filter[A](fa: List[A])(f: A => Boolean): List[A] = fa.filter(f) | |||
|
|||
override def foldM[G[_], A, B](fa: List[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | |||
Foldable.iteratorFoldM(fa.toIterator, z)(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we maybe do this one?
def step(in: (List[A], B])): G[Either[(List[A], B), A]] = in match {
case (Nil, b) => G.pure(Right(b))
case (a :: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
}
G.tailRecM((fa, z))(step)
right? If we had this, we could cover the common case for List
and NonEmptyList
, no?
@@ -118,9 +118,6 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances { | |||
|
|||
override def collect[A, B](fa: Stream[A])(f: PartialFunction[A, B]): Stream[B] = fa.collect(f) | |||
|
|||
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | |||
Foldable.iteratorFoldM(fa.toIterator, z)(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could copy the List
approach above here:
def step(in: (Stream[A], B])): G[Either[(Stream[A], B), A]] = in match {
case (Stream.empty, b) => G.pure(Right(b))
case (a #:: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
}
G.tailRecM((fa, z))(step)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patten match Stream into a #:: tail
will force an evaluation of the head of the tail
, so I used vanilla if
@@ -90,9 +90,6 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |||
|
|||
override def collect[A, B](fa: Vector[A])(f: PartialFunction[A, B]): Vector[B] = fa.collect(f) | |||
|
|||
override def foldM[G[_], A, B](fa: Vector[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | |||
Foldable.iteratorFoldM(fa.toIterator, z)(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector.tail
is so slow, we are probably better off doing fa.toList.foldM
here if we do the above.
@johnynek thanks for helping me with these, it was too lazy of me. I've updated the implemenations. |
@@ -118,8 +119,17 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances { | |||
|
|||
override def collect[A, B](fa: Stream[A])(f: PartialFunction[A, B]): Stream[B] = fa.collect(f) | |||
|
|||
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | |||
Foldable.iteratorFoldM(fa.toIterator, z)(f) | |||
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, but now again the default implementation of foldM
in Foldable
is not tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. I'll correct the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. I also added the original redundant tests back to test the stream override implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we would profit if had some machinery like mentioned in #1684.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasMikula I think we are still tested with Map
and Set
but I agree tests for overrides would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. I thought about that too.
@@ -431,6 +431,9 @@ private[data] sealed trait NonEmptyListInstances extends NonEmptyListInstances0 | |||
override def fold[A](fa: NonEmptyList[A])(implicit A: Monoid[A]): A = | |||
fa.reduce | |||
|
|||
override def foldM[G[_], A, B](fa: NonEmptyList[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | |||
G.flatMap(f(z, fa.head))(bnext => Foldable[List].foldM(fa.tail, bnext)(f)) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NonEmptyList
sort of gets this implementation already through NonEmptyReducible
, but I guess this saves us the split
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, forgot about that. A single split call probably doesn't justify the code duplication. I am going to remove this.
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = { | ||
def step(in: (Stream[A], B)): G[Either[(Stream[A], B), B]] = { | ||
val (s, b) = in | ||
if(s.isEmpty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scalastyle seems to want a space between the if
and the (
.
👍 |
@peterneyens I think addressed your feedback. Kindly take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kailuowang !
also related https://github.com/typelevel/cats/pull/1117/files#r123765689